-
Notifications
You must be signed in to change notification settings - Fork 1
Contact details only pages #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
6784148 to
a58db88
Compare
| Route( | ||
| "/cy/polling-stations/{postcode}/{uprn}", | ||
| endpoint=live_uprn_view, | ||
| "/cy/polling-stations/address/{postcode}/{uprn}", | ||
| endpoint=endpoints.election_information.live_uprn_view, | ||
| name="live_uprn_cy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing routes were a bit all over the shop. I tidied up the inconsistencies in fb21ec5 before adding more new route patterns.
Changing the sandbox routes doesn't really matter.
But this does change the direct URL to a UPRN in Welsh (English language pages not affected).
I think bringing this into line is right. I also think it is unlikely that it breaks useful deep links. However if you feel strongly we should maintain compatibility, I can add a redirect route for this.
| "/polling-stations", | ||
| endpoint=live_postcode_view, | ||
| endpoint=endpoints.election_information.live_postcode_view, | ||
| name="live_postcode_en", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every route needs to have a unique name.
I think if I was starting from scratch today, I would include election_information or something in the route names we already have to differentiate the two flavours of postcode form, postcode page, uprn page etc we now have.
For now, I've kept the existing names for the "election information" routes. If you want, I can go through and rename all the existing ones. The fiddly bit of this is making sure we've accounted for the renames everywhere we concatenate some string together to make a url_for()
| ), | ||
| ] | ||
|
|
||
| electoral_services_team_routes = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this PR introduces a bunch more URLs at the root of https://www.electoralcommission.org.uk (which could potentially conflict with URLs in their CMS, or might need CloudFlare changes to make sure the right requests get routed to us).
Obviously it is a breaking change to public URLs, but I wonder if we should just put all our stuff under a prefix? 🤔 Might make it easier to share a domain. Obviously that would require changing existing public URLs
| "/design-system", | ||
| endpoint=design_system_view, | ||
| name="design_system_view", | ||
| "/electoral-services/address/{postcode}/{uprn}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing site has this surprising behaviour where the UPRN has a {postcode} param in the route that basically functions as an ignored slug - it doesn't actually do anything.
So you can do stuff like:
- https://www.electoralcommission.org.uk/polling-stations/address/GL52HT/10010020128 (hopefully the postcode/result mismatch is obvious to you here)
- https://www.electoralcommission.org.uk/polling-stations/address/foobar/10010020128
which is confusing because what is on the page doesn't match the most human-readable bit of the URL.
I'd actually argue that this is essentially a bug and we should either
- Bin the
{postcode}route param or - Return a
400 Bad Requestif the{postcode}and{uprn}don't match
However I have decided to leave the worms inside that can in this PR.
What I have actually done in this PR is:
- Added the new UPRN route(s) with a
{postcode}URL param so the URLs under/polling-stations/and/electoral-services/follow the same format - Also completely ignored the
{postcode}URL param so the/electoral-services/routes are "bug for bug" compatible with the/polling-stations/routes
| name="static", | ||
| # Live, CY | ||
| Route( | ||
| "/cy/rwyf-yneg-pleidleisiwr/pleidleisiwr/electoral-services", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: need a translated slug for /electoral-services before this goes live
| # strip fields we don't need | ||
| fields = [ | ||
| "address_picker", | ||
| "addresses", | ||
| "electoral_services", | ||
| "registration", | ||
| "postcode_location", | ||
| ] | ||
| return {k: v for k, v in data.items() if k in fields} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..and then here I'm getting rid of everything other than the council contact details/address picker fields.
I haven't yet addressed the "value add" feature of what we do if there is some upcoming election in the user's area in this PR.
My gut instinct is that probably the way we handle that for the JSON case is: If dates[] is non-empty, we return a link to the corresponding "election information" page (with postcode/uprn pre-populated) rather than returning the candidates/polling station etc and trying to render that inline in the middle of a page about applying for a postal vote or whatever.
Happy to have a chat about that one.
Anyway, doing this doesn't preclude us from adding more fields (back) in later if we want
| {% extends request.base_template %} | ||
|
|
||
| {% block page_title %} | ||
| {% trans %}Your local council{% endtrans %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this is really merge-able we need to sort out the content, and then get translations for it.
There are some places where I have explicitly padded the page out with Lorem ipsum text.
There are also some places where I've put something in, but we should probably review it. For example, here I have made the header for this page "Your local council", but that's not really right. It might be your local council, or it might be The Electoral Office for Northern Ireland, or it might be your local council and also your local valuation joint board so we probably need to think of words that cover all 3 of those cases. Maybe "Your Local Electoral Services Team" is right?
Anyway, point is: All content is provisional and we need to work that out before we merge this.
| @@ -0,0 +1,50 @@ | |||
| function validate(js_strings) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I was working on this code and testing it, I realised a couple of things about the error handling here:
- If you don't have JS enabled, we just dump you back to the form with no error. Although there are some things we are doing that need JS, I feel like we should be able to handle the basic case (show an error if
invalid-postcode=1or whatever) using just server side code and HTML. - The "as you type" form validation works differently depending on whether you just landed on the page or if you previously entered an invalid postcode.
This PR is already quite big, so in the interests of not creeping scope, I have left that code as it is for now and just moved this to a different file so the "elections information" and "electoral services team" flows can share the same validation code.
As this PR stands, the error handling is no worse than it was before I started.
| "AA1 1AN": { | ||
| "description": "Senedd (2026 onwards) ballot. Party list election with multiple independent candidates", | ||
| "response": SENEDD_2026_ONWARDS_BALLOT, | ||
| }, | ||
| "AA1 1AP": { | ||
| "description": "Scottish parliament ballots: FPTP Constituency ballot and Party list Region ballot on same date. Also features different registration and electoral services contact details", | ||
| "response": SCOTTISH_PARLIAMENT_BALLOTS, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some more mock responses here.
This is partly just useful. Having mock responses covering the elections coming up this May is useful. It will help us with other bits of upcoming work. I also needed a mock response with both council and VJB for testing, so the Scotland one supports that.
I think there is a case for saying some of this stuff should actually be upstreamed to the response builder package but I've put it in here for now.
| @@ -0,0 +1,53 @@ | |||
| from app import app | |||
| from starlette.testclient import TestClient | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this project doesn't have great tests. I've added a few here anyway. Gotta start somewhere.
The case that isn't covered here is the address picker flow. In general, I don't think we've really figured out how to mock the address picker flow well with the response builder. I would like to crack that nut as it probably helps us in several places, but I decided not to do it in this PR.
a58db88 to
b5ab89a
Compare
This PR adds a flow for the user to get the details of their electoral services team independently of any information about elections.
It includes a user-facing flow using plain forms and HTML. It also includes JSON endpoints numiko can use to make an inline js-based flow.
I've ended up doing a bit of restructuring and refactoring work in here and adding a bunch of new test data so there is more to review here than I would like, but I've tried to avoid too much scope creep.
I will leave a copious amount of comments inline on the diff, as is my wont.